Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FOR DISCUSSION: Mods to make nyc work properly for bedrock modules. #8

Merged
merged 1 commit into from
Apr 15, 2020

Conversation

mattcollier
Copy link
Contributor

The good news is, we have A way of getting coverage on a Bedrock module.

Here's a report: https://codecov.io/gh/digitalbazaar/bedrock-profile-http/tree/e3f64b57bec39764f23303461b0860c2798bef7b/lib

The bad news is that nyc doesn't like working with stuff in node_modules generally and detests symlinks all-together.

See this: istanbuljs/nyc#1301 (comment)

I will highlight the specific changes in files.

@@ -3,7 +3,10 @@
"version": "0.0.1-0",
"private": true,
"scripts": {
"test": "node --preserve-symlinks test.js test"
"test": "NODE_PATH=./node_modules node --preserve-symlinks test.js test",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use NODE_PATH env var to tell node to use the node_modules folder in the test folder.

At first, this may seem terrible. However, it may solve an separate issue we've been having. This needs to be tested. That issue is that...

In order to get our linting stuff working for devs, we have to do an npm install on the root of the module, which installs all of the dependencies. This is partially due to the fact that doing npm install --only dev does not work with eslint. See: digitalbazaar/eslint-config-digitalbazaar#33

So, having dependencies in the node_modules in the root of the project can lead to confusion when devs need to symlink other deps for testing in the test framework. For instance, if a dev creates a symlink for test/node_modules/edv-client, node_modules/edv-client will still be used instead.

test/test.js Outdated
@@ -12,7 +12,8 @@ require('bedrock-kms-http');
require('bedrock-passport');
require('bedrock-permission');
require('bedrock-profile');
require('bedrock-profile-http');
// require('bedrock-profile-http');
require('..');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The worst part is this. Although we could continue adding the module to the test package file with the file:.. syntax, we would require it using a relative require.

I'm going to spend a bit more time attempting to see if nyc can be bent to our will here. But I'm not hopeful due to the nyc issue referenced in the PR description.

@mattcollier mattcollier merged commit 74ca4e7 into workflow Apr 15, 2020
@mattcollier mattcollier deleted the working_coverage branch April 15, 2020 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant